Skip to content
This repository has been archived by the owner on Apr 28, 2020. It is now read-only.

Add L2 Network RDP connection #218

Merged
merged 2 commits into from
Feb 28, 2019
Merged

Add L2 Network RDP connection #218

merged 2 commits into from
Feb 28, 2019

Conversation

rawagner
Copy link
Contributor

No description provided.

@rawagner
Copy link
Contributor Author

VM with L2 network and guest agent
l2_rdp

VM with L2 network without guest agent
no_guest

VM with no L2 network (there is no choice to choose L2 in dropdown)
no_l2

Connecting to VM via service
service_rdp

Connecting to VM via service, but no service is configured/found
no_service

@coveralls
Copy link

coveralls commented Feb 15, 2019

Pull Request Test Coverage Report for Build 855

  • 36 of 54 (66.67%) changed or added relevant lines in 5 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.6%) to 86.491%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/components/VmConsoles/VmConsoles.js 4 5 80.0%
src/utils/selectors.js 2 4 50.0%
src/components/VmConsoles/DesktopViewerSelector.js 24 39 61.54%
Totals Coverage Status
Change from base Build 665: -0.6%
Covered Lines: 2106
Relevant Lines: 2325

💛 - Coveralls

@rawagner
Copy link
Contributor Author

@matthewcarleton

@rawagner rawagner force-pushed the l2_rdp branch 2 times, most recently from 822743b to 2ba4923 Compare February 15, 2019 14:02
Copy link
Member

@fabiand fabiand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that the flow is user friendly. It's technology focused. not use-case driven.

@lizsurette
Copy link

I don't think that the flow is user friendly. It's technology focused. not use-case driven.

I think we could probably work the connection type into the dropdown we use to allow the user to connect a bit better. Maybe user a flyout in the dropdown to let them choose the connection type?

I do agree with @fabiand that maybe the terminology we use could be a bit more user friendly...would it be worth just allowing the user to connect "via Network" and list the networks to choose from? If there is only 1 network available then we could probably just do this by default without asking, right? I don't think the user would know what L2 is necessarily.

@fabiand Do you have any specific ideas in mind?

@andybraren
Copy link

andybraren commented Feb 18, 2019

I'm unfamiliar with the technical requirements here, but if the user sometimes needs to choose a Connection Type and NIC I think those could be pretty easily worked into the design we made a while back (part of the VM Details doc). Maybe the two dropdowns could be included below the "Console not connected" text?

6-1a-consoles-running

This old-ish design also put Desktop Client and Manual Connection details within a modal accessed via the "Remote connection settings" link button (so users can access them even if an inline console is showing), but I like the idea of showing those details within the page as you've done when it makes sense to do so.

6-3-2-settings-modal

@mareklibra
Copy link
Contributor

I think we could probably work the connection type into the dropdown we use to allow the user to connect a bit better. Maybe user a flyout in the dropdown to let them choose the connection type?

I like that, IIUC there would be single dropdown with:

Serice
Nic name 1
Nic name 2
...

Even if corresponding Service object can not be found, the option will be there and leading to additional info how to create it. Please note there is WIP to enable exposing VMs as services via GUI, this can be included. And even the Create VM flow (in GUI) can be enhanced to simplify that optional step.

@mareklibra
Copy link
Contributor

@andybraren , the presented design makes sense as well.
Anyway there was already quite a lot of effort invested to the recent implementation as it was created in tight cooperation with different designers (cross-product). Per-request, the reimplementation is in pf-react now, so potential (significant) change will cost another round of reviews (read: time).
So going this direction is feasible but will require agreement among multiple parties.

@jelkosz
Copy link
Contributor

jelkosz commented Feb 18, 2019

I think we could probably work the connection type into the dropdown we use to allow the user to connect a bit better. Maybe user a flyout in the dropdown to let them choose the connection type?

I like that, IIUC there would be single dropdown with:

Serice
Nic name 1
Nic name 2
...

+1, I like this option.
What others think? @fabiand would you find this user friendly?

@fabiand
Copy link
Member

fabiand commented Feb 18, 2019

@fabiand Do you have any specific ideas in mind?

As a user, when

  • a. connecting to RDP (or any port of the guest)
  • b. and there are multiple NICs on the guest

then

  • 1 we need to ask "How do you want to connect to the port? [selection of NICs]"
  • 2.a For pod network: Create a service for the selected (pod) NIC
  • 2.b For other networks: Connect to the IP of the selected NIC

Thus we do not need to ask for the connection type, as it can be inferred from the selected NIC. Instead we need to ask via which interface he would like to access RDP.

Please note: Because thex are external networks it is not guaranteed that the user (or webui) can actually reach this IP - because it might be living in a different/non-routable network segment.

@dankenigsberg @SchSeba is this correct?

@rawagner
Copy link
Contributor Author

rawagner commented Feb 19, 2019

multus network
1_multus

multus network - no guest agent installed
1_no_guest
multus network - guest agent is installed but ip address is not reported
1_no_ip
pod network - no service is configured/found ( there is already issue to be able to expose VM as service from UI kubevirt/web-ui#160 )
1_no_service
pod network
1_service

@SchSeba
Copy link

SchSeba commented Feb 19, 2019

@fabiand

Please note: Because thex are external networks it is not guaranteed that the user (or webui) can actually reach this IP - because it might be living in a different/non-routable network segment.
@dankenigsberg @SchSeba is this correct?

Correct secondary network can be unreachable from the webui (openshift api address)

@rawagner
Copy link
Contributor Author

@fabiand @lizsurette please take a look at latest version #218 (comment) and let me know what you think

@fabiand
Copy link
Member

fabiand commented Feb 20, 2019

@rawagner I like it much more. Just a nit: just looking at

  • as a user - I wonder what the UI asks me to do.

Instead of Network Interface: wouldn#t it make sense to (there or layout wise different) give some more context to the user, i.e.:

Select the network interface to be used for accessing the RDP console.

@jelkosz
Copy link
Contributor

jelkosz commented Feb 21, 2019

Instead of Network Interface: wouldn#t it make sense to (there or layout wise different) give some more context to the user, i.e.:

Select the network interface to be used for accessing the RDP console.

could be a question mark icon next to it. The text could be as you suggested: The network interface to be used for accessing the RDP console.

@rawagner
Copy link
Contributor Author

help added
1_help

@matthewcarleton
Copy link
Contributor

@rawagner this is looking great! I wonder if introducing "RDP console" in the tooltip is clear if we are not using that term any where else. Should we have "RDP (remote desktop protocol)" just to be clear?

@lizsurette
Copy link

It sounds like @fabiand still had some general usability concerns, mostly around terminology and layout, which I can totally understand. I think the addition of the information icon helps, but I'm wondering if we can further simplify.

A few additional thoughts:

  1. I like what @matthewcarleton has suggested around spelling out RDP to make it more clear.
  2. I still wonder if we could we bury the network selection into the "Console selection" as a flyout or multiple options. Where the user selects "Desktop Viewer", maybe we should extend those options to include something like "Desktop Viewer - accessed via Network 1" and "Desktop Viewer - accessed via Network 2". We could also shift the informational icon up to this higher selection in this case too to put some more context around it.

screen shot 2019-02-22 at 2 33 09 pm

3. If there is only one Network Interface for accessing the RDP console, could we just assume that and not present the Network Interface selection to the user?

screen shot 2019-02-22 at 2 29 58 pm

Any other thoughts, @andybraren?

Hopefully this helps and thanks to everyone for continuing to work on this to make it more and more clear with each revision :)

@fabiand
Copy link
Member

fabiand commented Feb 25, 2019

I like this new direction.

In the console drop down, we should eplicitly spell out the specific options ("RDP Console", "VNC Console", "Serial Console").

For "VNC Console" and "Serial Console" no user intervention is needed.

For "RDP Console" we can offer: "RDP Console - Accessed via net-1", "RDP Console - Accessed via net-1".

@rawagner
Copy link
Contributor Author

I agree that the Desktop Viewer is a bit confusing and I like having RDP Console directly in Consoles Dropdown however Consoles design comes from Patternfly where it was decided that we do not want to have RDP in console types but Desktop Viewer as Desktop Viewer component shows connections not only to RDP (Launch Remote Desktop button) but also VNC (Launch Remote Viewer button) and Spice (cc @mareklibra ). So if we want to change the design as suggested we need to change it in Patternfly. @lizsurette thoughts ?

As we are using latest Patternfly design I propose to merge this PR and address design concerns in followup issue open against Patternfly.

@mareklibra
Copy link
Contributor

Per-request, the consoles are located in pf-react to keep naming and design consistent across products.
I don't have strict opinion what exact name to use here, but it should make sense in other products as well.

It was decided to keep all desktop-based access under the single Desktop Viewer (changed from former Remote Desktop) which provides not only single-click access via downloading of a .vv or similar file but details for manual connection as well to support other than remote-viewer clients.

In context of the kubevirt-web-ui, the support for desktop based VNC clients is recently pending until we find reasonable way how to proxy API websocket via local tcp port. But this kind of access will be there as well.
We can hide the Launch Remote Viewer until fully implemented. Please note, this button provides acces to both VNC or SPICE in other products.

@fabiand
Copy link
Member

fabiand commented Feb 26, 2019

Thanks for the explanations.

@mareklibra mareklibra merged commit 5b94988 into kubevirt:master Feb 28, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants